-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow users to inject a postrenderer #219
Allow users to inject a postrenderer #219
Conversation
820887e
to
f067470
Compare
@joelanford pinging you for your review on this one 🙏 |
pkg/reconciler/reconciler_test.go
Outdated
objs := manifestToObjects(renderedManifests.String()) | ||
var manifests = make([]string, len(objs)) | ||
for i, obj := range objs { | ||
var annotations = obj.GetAnnotations() | ||
if annotations == nil { | ||
annotations = map[string]string{} | ||
} | ||
annotations["foo"] = "bar" | ||
obj.SetAnnotations(annotations) | ||
manifest, err := yaml.Marshal(obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I am aware that this is test code, and in this particular context speed does not matter much, but I'm making this point thinking about the use case that this change is designed to serve, i.e. adding annotations arbitrary modifications to all objects.
I'm concerned that this added round of deserialization/serialization will slow things down even more (compared to how slow helm-based operator already is).
Can you think of any other way that we could inject annotations to already-deserialized objects? Perhaps there is a different helm library hook that we could leverage? Or maybe we can ask for one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the ways we could do this is using the controller-runtime interceptor, intercept the create/update calls, and handle structured and unstructured objects and lists to append the annotations before creating or updating the objects. Users could also implement a custom MutatingWebHook to add annotations. Or maybe even update the charts themselves ?
Though in stackrox's desired usecase (apply user-provided overlay patches), this could only be done, AFAIK, using de & reserialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, it's for arbitrary modifications, not just annotations. I updated my comment above.
However given that this library already does a deserialize+reserialize in the ownerPostRenderer
I think it might be better to refactor the code there to allow adding custom visitors that could walk already parsed objects, rather than custom postrenderers that would repeat the same work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, because the user might want to remove parts of the manifest altogether, or add new resources to it, which would not be possible with a walk
.
I ran some benchmark tests, and the impact seems negligible. Deserializing then re-serializing an object from-to a yaml manifest takes roughly 0.0003 sec. I ran a test with 1000 deployments in 1 manifest (extremely large manifest), and it takes 0.27 sec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about visitor not being powerful enough for some use cases!
However I still think this change would be exposing functionality at the wrong level of abstraction.
It looks like the PostRenderer
hook exposed in the helm library works on the level of bytes.Buffer
objects because this is a natural fit for exposing it from the helm
CLI - since that works on the level of command pipelines.
While this works, and the overhead seems manageable (thanks for checking this!), I think it forces the API user to do unnecessary work, so is not elegant.
What do you think about exposing a kube.ResourceList
instead? (That one has Append
and Filter
, for example).
fc4a4aa
to
f11cb2f
Compare
pkg/reconciler/reconciler_test.go
Outdated
objs := manifestToObjects(renderedManifests.String()) | ||
var manifests = make([]string, len(objs)) | ||
for i, obj := range objs { | ||
var annotations = obj.GetAnnotations() | ||
if annotations == nil { | ||
annotations = map[string]string{} | ||
} | ||
annotations["foo"] = "bar" | ||
obj.SetAnnotations(annotations) | ||
manifest, err := yaml.Marshal(obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about visitor not being powerful enough for some use cases!
However I still think this change would be exposing functionality at the wrong level of abstraction.
It looks like the PostRenderer
hook exposed in the helm library works on the level of bytes.Buffer
objects because this is a natural fit for exposing it from the helm
CLI - since that works on the level of command pipelines.
While this works, and the overhead seems manageable (thanks for checking this!), I think it forces the API user to do unnecessary work, so is not elegant.
What do you think about exposing a kube.ResourceList
instead? (That one has Append
and Filter
, for example).
4fafe80
to
8ac34c0
Compare
Reopening this to run the CI again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good and make sense to me. Approving but pending merge based on resolving CI failures
@ludydoo could you please fix the linter errors as mentioned in the CI so that we can merge this |
Pull Request Test Coverage Report for Build 5748961402
💛 - Coveralls |
Done :) |
It would be useful for us to be able to inject a per-object PostRenderer to implement a feature such as istio overlays. see https://istio.io/latest/docs/setup/additional-setup/customize-installation/#patching-the-output-manifest
We cannot use the
actionClientConfigGetter
oractionClientGetter
with aWithInstallOption(...)
because this method doesn't pass theCustomResource
to the renderer at the moment of installing/upgrading/dry-running. Though, our usecase would be that each CustomResource would have configurable "patches" that would be evaluated against the manifest at the moment of reconciliation.Example of usecase:
https://github.com/stackrox/stackrox/blob/1fd296314419b25df5831e472e74512bd5e01e5a/operator/apis/platform/v1alpha1/central_types.go#L62C3-L62C3
https://github.com/stackrox/stackrox/blob/1fd296314419b25df5831e472e74512bd5e01e5a/operator/apis/platform/v1alpha1/overlay_types.go#L17
https://github.com/stackrox/stackrox/blob/ROX-18085-operator-overlays/operator/pkg/overlays/postrenderer.go
https://github.com/stackrox/stackrox/blob/1fd296314419b25df5831e472e74512bd5e01e5a/operator/pkg/reconciler/reconciler_factory.go#L82